-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Cypress Auth Test to POM and Improve Test Stability #9024
base: develop
Are you sure you want to change the base?
Refactor Cypress Auth Test to POM and Improve Test Stability #9024
Conversation
WalkthroughThe changes involve the deletion of two Cypress test files, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginPage
participant AuthService
participant NotificationService
User->>LoginPage: Enter credentials
LoginPage->>AuthService: Validate credentials
alt Successful login
AuthService-->>LoginPage: Success
LoginPage->>User: Redirect to dashboard
else Failed login
AuthService-->>LoginPage: Failure
LoginPage->>NotificationService: Show error message
NotificationService-->>User: Display error
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for care-ohc failed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
cypress/e2e/homepage_spec/UserLogin.cy.ts (2)
1-2
: Add TypeScript type annotations for better maintainability.While the code follows POM best practices, adding type annotations would improve maintainability and IDE support.
-import LoginPage from "pageobject/Login/LoginPage"; -const loginPage = new LoginPage(); +import { LoginPage } from "pageobject/Login/LoginPage"; +const loginPage: LoginPage = new LoginPage();
1-45
: Consider adding test cases for edge scenarios.The test suite has good basic coverage, but consider adding these scenarios:
- Session timeout handling
- Network error scenarios
- Concurrent login attempts
- Browser refresh during login process
- Password complexity validation
Also, consider creating custom commands for common assertions to improve code reuse.
Would you like me to help create these additional test cases or custom commands?
cypress/pageobject/Login/LoginPage.ts (4)
16-23
: LGTM! Consider adding type safety for credentials.Good implementation of parameterized login for both successful and failed scenarios. The boolean parameter effectively controls the test flow.
Consider creating an interface for credentials to make the code more maintainable:
interface LoginCredentials { username: string; password: string; } const VALID_CREDENTIALS: LoginCredentials = { username: "devdistrictadmin", password: "Coronasafe@123" }; const INVALID_CREDENTIALS: LoginCredentials = { username: "devdistrictadmin", password: "Corona" };
42-44
: Consider adding API verification for sign out.Good implementation using the helper function. However, consider adding API interception to verify the sign-out request completes successfully.
Add API verification similar to other methods:
interceptSignOutReq(): void { cy.intercept("POST", "**/api/v1/auth/logout").as("signOut"); } verifySignOutReq(): void { cy.wait("@signOut").its("response.statusCode").should("eq", 200); }
62-84
: Consider parameterizing expected status codes.Good implementation of API interception and verification. However, the expected status codes could be parameterized for more flexible testing.
Consider this approach:
interface ApiVerification { alias: string; expectedStatus: number; } verifyApiResponse({ alias, expectedStatus }: ApiVerification): void { cy.wait(`@${alias}`).its("response.statusCode").should("eq", expectedStatus); }This would allow more flexible status code verification while reducing code duplication.
Stability improvements need attention - hardcoded waits found
Multiple files still contain hardcoded waits that should be replaced with proper API interceptions and assertions:
PatientLogupdate.ts
: Contains 5 instances of hardcoded waits (2000-3000ms)PatientFileupload.ts
: Contains 3 instances including a 5000ms waitUsersManage.cy.ts
: Contains 3 instances of 5000ms waitsPatientConsultation.ts
: Contains waits of 5000ms and 3000msPatientRegistration.cy.ts
: Contains 6 instances of hardcoded waitsWhile API interceptions are well implemented in the LoginPage and other files, the presence of numerous hardcoded waits indicates that the stability improvements are incomplete. These should be replaced with proper API interceptions and assertions for better test reliability.
🔗 Analysis chain
Line range hint
1-84
: Verify test stability improvements.The changes align well with the PR objectives, implementing POM pattern and improving stability through API verification. Let's verify the stability improvements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining hardcoded waits in test files echo "Checking for hardcoded waits..." rg "cy\.wait\(\d+" cypress/ # Check for consistent use of API interception echo "Checking API interception usage..." rg "cy\.intercept" cypress/Length of output: 9813
src/components/Auth/Login.tsx (1)
Line range hint
26-427
: Consider splitting the component for better maintainability.The component currently handles both login and forgot password flows. Consider splitting it into separate components:
LoginForm.tsx
: Handle login flowForgotPasswordForm.tsx
: Handle password recovery flowAuthLayout.tsx
: Handle common layout elementsThis would improve:
- Code maintainability
- Component testing
- Reusability
- Separation of concerns
Additionally, consider extracting form validation logic into a custom hook for reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cypress/e2e/auth_spec/ForgotPassword.cy.ts
(0 hunks)cypress/e2e/auth_spec/auth.cy.ts
(0 hunks)cypress/e2e/homepage_spec/UserLogin.cy.ts
(1 hunks)cypress/pageobject/Login/LoginPage.ts
(2 hunks)src/components/Auth/Login.tsx
(3 hunks)
💤 Files with no reviewable changes (2)
- cypress/e2e/auth_spec/ForgotPassword.cy.ts
- cypress/e2e/auth_spec/auth.cy.ts
🔇 Additional comments (4)
cypress/pageobject/Login/LoginPage.ts (1)
29-29
: LGTM! Consistent use of submitButton command.
The change aligns with other login methods and improves test stability.
src/components/Auth/Login.tsx (3)
339-339
: LGTM: Semantic ID for forgot password button.
The added ID follows clear naming conventions and supports the test automation objectives.
375-375
: LGTM: Clear ID for navigation button.
The ID addition maintains consistency with the naming pattern and clearly indicates the button's purpose.
409-409
: LGTM: Descriptive ID for form submission button.
The ID addition completes the set of button identifiers needed for test automation while maintaining naming consistency.
describe("Authorisation/Authentication", () => { | ||
beforeEach(() => { | ||
cy.awaitUrl("/", true); | ||
}); | ||
|
||
it("Try login as admin with correct password", () => { | ||
loginPage.loginManuallyAsDistrictAdmin(); | ||
loginPage.interceptFacilityReq(); | ||
loginPage.verifyFacilityReq(); | ||
loginPage.ensureLoggedIn(); | ||
loginPage.clickSignOutBtn(); | ||
loginPage.verifyLoginPageUrl(); | ||
}); | ||
|
||
it("Try login as admin with incorrect password", () => { | ||
loginPage.interceptLoginReq(); | ||
loginPage.loginManuallyAsDistrictAdmin(false); | ||
loginPage.verifyLoginReq(); | ||
cy.verifyNotification("No active account found with the given credentials"); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance test stability with retry and error handling.
While the test structure is good, consider these improvements for better stability:
- Add retry mechanism for flaky scenarios
- Implement proper error handling for API failures
describe("Authorisation/Authentication", () => {
beforeEach(() => {
cy.awaitUrl("/", true);
});
- it("Try login as admin with correct password", () => {
+ it("Try login as admin with correct password", { retries: 2 }, () => {
loginPage.loginManuallyAsDistrictAdmin();
- loginPage.interceptFacilityReq();
+ // Add timeout and error handling for API calls
+ loginPage.interceptFacilityReq({ timeout: 10000 }).catch((err) => {
+ cy.log('Facility request failed:', err);
+ throw err;
+ });
loginPage.verifyFacilityReq();
loginPage.ensureLoggedIn();
loginPage.clickSignOutBtn();
loginPage.verifyLoginPageUrl();
});
- it("Try login as admin with incorrect password", () => {
+ it("Try login as admin with incorrect password", { retries: 2 }, () => {
loginPage.interceptLoginReq();
loginPage.loginManuallyAsDistrictAdmin(false);
loginPage.verifyLoginReq();
cy.verifyNotification("No active account found with the given credentials");
});
});
Committable suggestion skipped: line range outside the PR's diff.
describe("Forgot Password", () => { | ||
const userName = "dummy_user_1"; | ||
beforeEach(() => { | ||
cy.awaitUrl("/", true); | ||
cy.verifyAndClickElement("#forgot-pass-btn", "Forgot password?"); | ||
}); | ||
|
||
it("Send Password Reset Link", () => { | ||
loginPage.fillUserNameInForgotPasswordForm(userName); | ||
loginPage.interceptResetLinkReq(); | ||
loginPage.clickSendResetLinkBtn(); | ||
loginPage.verifyResetLinkReq(); | ||
cy.verifyNotification("Password Reset Email Sent"); | ||
}); | ||
|
||
it("Go to Login page", () => { | ||
loginPage.clickBackButton(); | ||
loginPage.verifyLoginPageUrl(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the analysis of the codebase, I can now provide the final response:
Consider using fixtures for test data and add validation coverage
The review comment is valid as the codebase shows:
- Most test data is currently hardcoded as constants across test files
- Limited input validation in test cases
- No negative test cases for the forgot password flow
However, the suggested implementation needs adjustment:
- Instead of importing from
testData.json
, create a dedicated fixture file for auth-related test data - Add validation for both success and error scenarios
- Include rate limiting test only if the API supports it (needs manual verification)
+// cypress/fixtures/auth.json
+{
+ "forgotPassword": {
+ "validUser": "dummy_user_1",
+ "invalidUser": "nonexistent@user",
+ "invalidFormat": "user@"
+ }
+}
describe("Forgot Password", () => {
+ beforeEach(() => {
+ cy.fixture('auth.json').as('authData');
+ cy.awaitUrl("/", true);
+ cy.verifyAndClickElement("#forgot-pass-btn", "Forgot password?");
+ });
- const userName = "dummy_user_1";
-
it("Send Password Reset Link", () => {
- loginPage.fillUserNameInForgotPasswordForm(userName);
+ cy.get('@authData').then((authData) => {
+ loginPage.fillUserNameInForgotPasswordForm(authData.forgotPassword.validUser);
+ cy.get('input[name="username"]')
+ .should('have.value', authData.forgotPassword.validUser)
+ .and('be.valid');
+ cy.get('button[type="submit"]').should('be.enabled');
+ loginPage.interceptResetLinkReq();
+ loginPage.clickSendResetLinkBtn();
+ loginPage.verifyResetLinkReq();
+ cy.verifyNotification("Password Reset Email Sent");
+ });
});
+ it("Show error for invalid username format", () => {
+ cy.get('@authData').then((authData) => {
+ loginPage.fillUserNameInForgotPasswordForm(authData.forgotPassword.invalidFormat);
+ cy.get('input[name="username"]').should('have.class', 'error');
+ cy.get('button[type="submit"]').should('be.disabled');
+ });
+ });
🔗 Analysis chain
Move test data to fixtures and add input validation.
Consider these improvements:
- Move test data to a fixture file
- Add validation for the username input
- Add assertions for button states
+import testData from '../fixtures/testData.json';
+
describe("Forgot Password", () => {
- const userName = "dummy_user_1";
beforeEach(() => {
cy.awaitUrl("/", true);
cy.verifyAndClickElement("#forgot-pass-btn", "Forgot password?");
});
it("Send Password Reset Link", () => {
- loginPage.fillUserNameInForgotPasswordForm(userName);
+ loginPage.fillUserNameInForgotPasswordForm(testData.users.testUser.username);
+ // Validate input field
+ cy.get('input[name="username"]').should('have.value', testData.users.testUser.username);
+ // Verify button is enabled
+ cy.get('button[type="submit"]').should('be.enabled');
loginPage.interceptResetLinkReq();
loginPage.clickSendResetLinkBtn();
loginPage.verifyResetLinkReq();
cy.verifyNotification("Password Reset Email Sent");
});
Also, consider adding negative test cases:
- Invalid username format
- Non-existent username
- Rate limiting scenarios
Let me check the implementation of these methods and validation patterns in other test files to make a more informed decision.
Let me check for existing test data patterns and negative test cases to make a complete assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if similar validation exists in other test files
rg -l "fillUserNameInForgotPasswordForm|verifyNotification" --type ts
Length of output: 1074
Script:
#!/bin/bash
# Check implementation of fillUserNameInForgotPasswordForm and related validations
rg "fillUserNameInForgotPasswordForm" -A 5 --type ts
# Check if test data is used from fixtures in other tests
rg "fixtures.*\.json" --type ts
# Check for input validation patterns in other tests
ast-grep --pattern 'cy.get($SELECTOR).should("have.value", $_)'
Length of output: 1589
Script:
#!/bin/bash
# Check for existing test data patterns
rg "const \w+ = ['\"].*['\"]" cypress/e2e/ --type ts
# Check for negative test cases and validation patterns
rg "it\(.*invalid.*|it\(.*error.*" -A 5 cypress/e2e/ --type ts
# Check for existing fixture usage patterns
fd fixtures.ts$ -x cat {}
Length of output: 21386
verifyLoginPageUrl(): void { | ||
cy.url().should("include", "/"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance URL verification specificity.
The current URL verification is too broad. Consider checking for a more specific path.
- verifyLoginPageUrl(): void {
- cy.url().should("include", "/");
+ verifyLoginPageUrl(): void {
+ cy.url().should("include", "/login");
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
verifyLoginPageUrl(): void { | |
cy.url().should("include", "/"); | |
} | |
verifyLoginPageUrl(): void { | |
cy.url().should("include", "/login"); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
cypress/pageobject/Login/LoginPage.ts (2)
16-23
: Consider externalizing test credentials.While the implementation is good, consider moving the hardcoded credentials to a configuration file or environment variables for better maintainability and security.
Example structure:
// cypress/fixtures/credentials.json { "districtAdmin": { "username": "devdistrictadmin", "password": "Coronasafe@123", "invalidPassword": "Corona" } }
42-60
: Enable method chaining for better test flow.Consider returning
this
from void methods to enable method chaining, which can make tests more readable and fluent.Example refactor:
- clickSignOutBtn(): void { + clickSignOutBtn(): this { cy.verifyAndClickElement("#sign-out-button", "Sign Out"); + return this; } - fillUserNameInForgotPasswordForm(userName: string): void { + fillUserNameInForgotPasswordForm(userName: string): this { cy.get("#forgot_username").type(userName); + return this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/pageobject/Login/LoginPage.ts
(2 hunks)src/components/Auth/Login.tsx
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Auth/Login.tsx
interceptFacilityReq(): void { | ||
cy.intercept("GET", "**/api/v1/facility/**").as("getFacilities"); | ||
} | ||
|
||
verifyFacilityReq(): void { | ||
cy.wait("@getFacilities").its("response.statusCode").should("eq", 200); | ||
} | ||
|
||
interceptLoginReq(): void { | ||
cy.intercept("POST", "**/api/v1/auth/login").as("userLogin"); | ||
} | ||
|
||
verifyLoginReq(): void { | ||
cy.wait("@userLogin").its("response.statusCode").should("eq", 401); | ||
} | ||
|
||
interceptResetLinkReq(): void { | ||
cy.intercept("POST", "**/api/v1/password_reset").as("resetLink"); | ||
} | ||
|
||
verifyResetLinkReq(): void { | ||
cy.wait("@resetLink").its("response.statusCode").should("eq", 200); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance API interception methods for better flexibility and type safety.
The API interception methods could be improved in several ways:
- Use constants for status codes and URL patterns
- Add flexibility for different response scenarios
- Implement type safety for response handling
Example implementation:
// Constants
const API_ENDPOINTS = {
FACILITY: '**/api/v1/facility/**',
LOGIN: '**/api/v1/auth/login',
PASSWORD_RESET: '**/api/v1/password_reset'
} as const;
const HTTP_STATUS = {
OK: 200,
UNAUTHORIZED: 401
} as const;
// Enhanced methods
interface ApiResponse {
statusCode: number;
body?: unknown;
}
interceptLoginReq(response?: Partial<ApiResponse>): this {
cy.intercept('POST', API_ENDPOINTS.LOGIN, response).as('userLogin');
return this;
}
verifyLoginReq(expectedStatus: number = HTTP_STATUS.UNAUTHORIZED): this {
cy.wait('@userLogin')
.its('response.statusCode')
.should('eq', expectedStatus);
return this;
}
Hey @nihal467 can you please review this PR |
Proposed Changes
auth_spec
tohomepage_spec
andauth.cy.ts
toUserLogin.cy.ts
.auth.cy.ts
andforgotpassword.cy.ts
intoUserLogin.cy.ts
, organizing them under separatedescribe
blocks.LoginPage.ts
for login credentials.cy.intercept
before accessing#user-profile-name
to ensure page load.UserProfilePage.ts
andcommands.ts
.cy.intercept
for stability.Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores